i18n Banner #46

mateoclarke · 2017-12-21T00:20:55Z

Addresses part of #21. Punts on most of #35

This isn't totally functional yet. Just mainly focusing on the markup. The language choice doesn't persist and it is only stored in the state of this component when it should probably be managed at a higher level.

We still need to:

update the API call to pull down language appropriate data onto the page
and/or we need to reroute the user to a url with with the language code appended, ex: austin.gov/es
The dropdown isn't a great UX, but I wanted to talk to Celine about what she expects from it before spending more time on it. edit: now remembering about Language Selection #35 where its better defined

Not sure about the best long term approach there so I'm going to punt until we can investigate if Next.js has some patterns we should look into.

I used this method for SVG insertion https://css-tricks.com/creating-svg-icon-system-react/

# Conflicts: # src/components/Footer.js # src/css/components/Footer.scss

benweatherman

Code looks 👌 to me. Thanks for posting the screenshots.

I feel like the URL redirect is a best practice as a long term solution (some people on stackexchange say so). Using a URL would allow us to detect the language the first time a user visits the site and do a redirect from the server. Also forgive me if you weren't looking for feedback on that stuff yet.

benweatherman · 2017-12-21T05:56:47Z

src/components/I18nBanner.js

+  constructor(props) {
+    super(props);
+    this.state = {
+      activeLanguage: locale().substring(0,2) || 'en', // we could


benweatherman · 2017-12-21T05:59:29Z

src/components/I18nBanner.js

+      activeLanguage: languageAbbr,
+      isOpen: false,
+    })
+    console.log(this.state.activeLanguage)


Definitely not something for right now: if/when we do server redirects for language we should set a cookie here so the server will know the user has overridden the language from the browser settings.

ifsimicoded

HOLIDAAAAAYS!!!

ifsimicoded · 2017-12-21T22:14:46Z

src/components/I18nBanner.js

+  }
+
+  render() {
+    const displayedLanguageAmount = 4


really minor, but since the number of shown languages doesn't change on each render, you can create a method that sets this.primaryLanguages and this.secondaryLanguages in the constructor. and then reference those two class vars in render.

cool, that makes sense.

ifsimicoded · 2017-12-21T22:19:48Z

src/css/components/I18nBanner.scss

@@ -0,0 +1,74 @@
+.coa-I18nBanner {


Can we rename this _I18nBanner.scss so it's treated as a partial. This will fix the missing scss var issues.

ifsimicoded · 2017-12-21T22:20:46Z

src/css/components/I18nBanner.scss

+.coa-I18nBanner {
+  width: 100%;
+  background-color: $color-gray-light;
+  padding: .7rem 0;


can we not use magic numbers and stick to the padding dimensions we have? $coa-space-level1 or $coa-space-level2?

ifsimicoded · 2017-12-21T22:21:10Z

src/css/components/I18nBanner.scss

+
+.coa-I18nBanner__prompt {
+  display: none;
+  font-size: 1.5rem;


same as above, can we stick to using variables for defining sizes?

cool. Do we want to define these in _variables_coa.scss? For now, I'm using the font size values set in _variables_uswds.scss

ifsimicoded · 2017-12-21T22:29:09Z

src/components/I18nBanner.js

+    };
+  }
+
+  languageClassName = (language) => {


SUUUUPER minor, can we update these functions to getLanguageClassName and getSecondaryLanguageMenuClassName? I like the action the function is performing to be REAAAALLLLY STUPID obvious.

ifsimicoded · 2017-12-21T23:10:08Z

src/components/I18nBanner.js

+                        onClick={this.setLangauage} lang={language.abbr}
+                        key={i}
+                      >
+                        <span className="coa-I18nBanner__language--hide-sm">{language.title}</span>


instead of creating specific hide-sm classes can we create generic ones for breakpoints.
.hidden--sm --> display none for small width screens and higher
.hidden--md --> display none for medium width screens and higher
.visible--sm --> display none by default, display initial for small screens and higher
.visible--md --> display none by default, display initial for medium screens and higher
We can use our screen size variables to build these helpers.

yup, I knew I was taking a step in that direction but was hesitant to commit to it. Thanks for calling it out!

So as I started digging into how to create these helper classes, I thought it was pretty interesting how they are doing it now in Bootstrap v4:
Code: https://github.com/twbs/bootstrap/blob/7b766e1ad53b0c22de42dac04d2472f287334e2a/scss/utilities/_display.scss
Docs: https://getbootstrap.com/docs/4.0/utilities/display/#hiding-elements

Should we move in that direction?

ifsimicoded · 2017-12-21T23:13:21Z

src/components/I18nBanner.js

+      <div className="coa-I18nBanner">
+        <div className="wrapper">
+          <div className="row">
+            <div className="col-md-4 col-xs coa-I18nBanner__prompt">


Can we try to nest things inside of grid classes so display properties and margin properties and stuff don't conflict.
so ... <div className="col-md-4 col-xs>.....

ifsimicoded · 2017-12-21T23:54:04Z

src/css/components/I18nBanner.scss

+}
+
+.coa-I18nBanner__prompt {
+  display: none;


can we use the visible--md helper instead of defining specific styles for hiding.

ifsimicoded · 2017-12-21T23:54:27Z

src/svg/CaretDown.js

+  const { size } = props
+
+  return (
+    <svg width={size} height={size} viewBox="0 0 1792 1792" xmlns="http://www.w3.org/2000/svg" aria-labelledby="title">


SHOW ME HOW TO MAKE SVG PEEEEEAS!

remind me when we are both back in the office. no magic here.

ifsimicoded · 2017-12-22T00:08:13Z

src/components/I18nBanner.js

+  setLangauage = (e) => {
+    e.stopPropagation();
+    e.nativeEvent.stopImmediatePropagation();
+    const languageAbbr = e.target.parentElement.lang;


this way of getting the element that's clicked seems highly coupled to the dom structure. maybe try e.currentTarget to get the element which had the handler.

yeah, great point. I was trying to accommodate both the main list and secondary list markup which had a small difference in DOM structure. But you're right that this is brittle and would break if someone refactored the HTML and not this line 60. I'm changing the lang="foo" attribute to live directly on the span instead of the parent li

# Conflicts: # src/App.js # src/css/App.scss

Mateo Clarke added 7 commits December 15, 2017 11:59

Set up footer

a13482a

placeholder for homepage

f986c4f

Add links and SVGs

2f1e494

I used this method for SVG insertion https://css-tricks.com/creating-svg-icon-system-react/

Merge branch 'master' into header

1e1f77b

# Conflicts: # src/components/Footer.js # src/css/components/Footer.scss

Add semi-functional i18n banner

b1d032d

make it more responsive

775711c

update language list

665e755

mateoclarke requested review from ifsimicoded and benweatherman December 21, 2017 00:22

benweatherman approved these changes Dec 21, 2017

View reviewed changes

could we?

a0ac395

This was referenced Dec 21, 2017

Language Selection #35

Closed

Header/Footer #21

Closed

ifsimicoded reviewed Dec 22, 2017

View reviewed changes

Mateo Clarke added 4 commits December 22, 2017 22:33

Merge branch 'master' into header

fae7ead

Merge branch 'master' into header

4b6d4bf

# Conflicts: # src/App.js # src/css/App.scss

move around css file and include banner in app layout

9982039

PR feedback

485ae48

mateoclarke merged commit 06b34ef into master Dec 29, 2017

mateoclarke deleted the header branch February 8, 2018 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i18n Banner #46

i18n Banner #46

mateoclarke commented Dec 21, 2017 •

edited

Loading

benweatherman left a comment

benweatherman Dec 21, 2017

mateoclarke Dec 21, 2017

benweatherman Dec 21, 2017

mateoclarke Dec 29, 2017

ifsimicoded left a comment

ifsimicoded Dec 21, 2017

mateoclarke Dec 29, 2017

ifsimicoded Dec 21, 2017

mateoclarke Dec 29, 2017

ifsimicoded Dec 21, 2017

mateoclarke Dec 29, 2017

ifsimicoded Dec 21, 2017

mateoclarke Dec 29, 2017

ifsimicoded Dec 21, 2017

mateoclarke Dec 29, 2017

ifsimicoded Dec 21, 2017

mateoclarke Dec 29, 2017

mateoclarke Dec 29, 2017

ifsimicoded Dec 21, 2017

mateoclarke Dec 29, 2017

ifsimicoded Dec 21, 2017

mateoclarke Dec 29, 2017

ifsimicoded Dec 21, 2017

mateoclarke Dec 29, 2017

ifsimicoded Dec 22, 2017

mateoclarke Dec 29, 2017

i18n Banner #46

i18n Banner #46

Conversation

mateoclarke commented Dec 21, 2017 • edited Loading

benweatherman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifsimicoded left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateoclarke commented Dec 21, 2017 •

edited

Loading